content: Include inline styles (italic etc.) in what we pass to WidgetSpans #1821
Conversation
There was a problem hiding this comment.
Thanks for doing this @chrisbobbe! LGTM, couple of small comments below.
lib/widgets/katex.dart
Outdated
| // Remove link color, but | ||
| // TODO(design) do we want to do that? | ||
| color: baseColor, | ||
| // italic and strikethrough are removed below, but |
There was a problem hiding this comment.
Hmm, I did a quick test and it looks like strikethrough seems to work:
There was a problem hiding this comment.
Ah yeah that's right:
- The strikethrough is not applied to the katex in the WidgetSpan
- …but it is applied to an ancestor span, and the inline-katex implementation doesn't obscure it by setting an opaque background :) so it shows through
lib/widgets/katex.dart
Outdated
| // Just for completeness, remove "wght", but it wouldn't do anything anyway | ||
| // since KaTeX_Main is not a variable-weight font. | ||
| fontVariations: [], | ||
| // Remove link color, but |
There was a problem hiding this comment.
Looks like web client currently applies the link color over inline TeX, so we probably want to handle it similarly.
There was a problem hiding this comment.
Filed #1823 for that, so probably add a TODO pointing to that.
In the inline-math case, we're about to pass the actual ambient text style, which may have italic, strikethrough, bold, or link formatting. Consolidate the code that overrides each of those, so that it reads as intentional, and add TODOs for overrides that we might not want to keep.
…tSpans Fixes zulip#1813. Fixes zulip#1819. This is NFC for inline math, since all the inline styles are overridden there. But see the previous commit for where we consolidated those overrides, making a natural place to add more if needed in the future, with TODOs for some existing ones we might not want to keep.
chrisbobbe
force-pushed
the
pr-widget-span-not-getting-inline-styles-fix
branch
from
f24d250 to
8310d37
Compare
|
Thanks! Revision pushed, expanding the comment about strikethrough and adding a commit for #1823. Here are screenshots before and after that new commit:
|
rajveermalviya
added
integration review
There was a problem hiding this comment.
Interesting! Thanks @chrisbobbe for making these fixes, and @rajveermalviya for the previous reviews. Comments below.
| child: UserMention(ambientTextStyle: widget.style, node: node)); | ||
| child: UserMention(ambientTextStyle: _resolveStyleStack(), node: node)); |
There was a problem hiding this comment.
The contrast between _resolveStyleStack here, and widget.style in other places like for UnicodeEmojiNode and InlineCodeNode, is confusing on first look.
I puzzled over it for a few minutes, basically until I remembered your recent other PR #1820. IIUC, the point is that for a TextSpan, its styles will get automatically merged with those of its ancestors; but a WidgetSpan makes a barrier to that automatic merging.
So that'd be good to clarify. As doc on _resolveStyleStack if nothing else; and maybe add a getter for widget.style, and name that and rename _resolveStyleStack so that they're visibly parallel and the names help indicate why there's alternatives.
| @@ -1145,11 +1156,11 @@ class _InlineContentBuilder { | |||
| : WidgetSpan( | |||
| alignment: PlaceholderAlignment.baseline, | |||
| baseline: TextBaseline.alphabetic, | |||
| child: KatexWidget(textStyle: widget.style, nodes: nodes)); | |||
| child: KatexWidget(ambientTextStyle: _resolveStyleStack(), nodes: nodes)); | |||
There was a problem hiding this comment.
This makes me a little nervous performance-wise. What if there's a paragraph with 40 different inline math spans in it? (Imagine they're all short, like "x", "a^2", "y = 1", "\sin x".)
I guess it only matters if the whole thing is inside a styled span. But that's not uncommon in math texts: the whole statement of a theorem is commonly set in italics.
Maybe the first thing to do is to just measure, in a profile build, how long it takes to build a paragraph like that. If it's negligible, then great.
If that's more than negligible, then a good solution could be to have _resolveStyleStack memoize the result.
| @@ -1081,10 +1081,21 @@ class _InlineContentBuilder { | |||
| _recognizer = _recognizerStack!.removeLast(); | |||
| } | |||
|
|
|||
| InlineSpan _buildNodes(List<InlineContentNode> nodes, {required TextStyle? style}) { | |||
| final List<TextStyle> _styleStack = []; | |||
There was a problem hiding this comment.
The other potential performance worry here is that we're now allocating these lists for every paragraph, even trivial short paragraphs with no formatting in them.
So that'd also be good to measure the impact of.
If it's nontrivial, then a likely fix would be:
- Leave
widget.styleout of the list; only add styles from within the node tree. - Make the list nullable, and allocate lazily. (Just like _recognizerStack.)
Fixes #1813.
Fixes #1819.
Fixes #1823.
I've left out further testing because we have other pressing priorities; please let me know if something's missing or not well-explained.